Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add wrapper types for all geometries #78

Merged
merged 26 commits into from
Mar 27, 2023
Merged

Add wrapper types for all geometries #78

merged 26 commits into from
Mar 27, 2023

Conversation

rafaqz
Copy link
Member

@rafaqz rafaqz commented Nov 20, 2022

This PR adds wrapper types for all geometries.

The idea is these will work with wrapping

  1. Any other GeoInterface.jl compatible objects, as-is.
  2. AbstractArrays of e.g. PointTrait objects for LineString or MultiPoint.

Basically you should be able to hack together any geometries you want using any package components and they will work.

It's not quite there yet.

@rafaqz rafaqz marked this pull request as ready for review January 16, 2023 17:00
@rafaqz
Copy link
Member Author

rafaqz commented Jan 16, 2023

@evetion if you want to have a look at this it would be good to get it merged soon so we can use it in tests.

There are still a few types left to test but comments on overall structure would be good at this stage.

The idea is geometry constructors and methods are all defined in an @eval loop so they all behave the same way, just with varied levels of nesting and diferent child types/traits. Point is defined on its own as it's quite different.

@rafaqz
Copy link
Member Author

rafaqz commented Jan 22, 2023

This is ready to go

@rafaqz
Copy link
Member Author

rafaqz commented Jan 28, 2023

@evetion @visr can I get a review for this? It's needed to finish the LibGEOS convert PR.

A question I have is: should the types go in a module they can be easily imported, instead of all exported

src/fallbacks.jl Outdated Show resolved Hide resolved
src/fallbacks.jl Outdated Show resolved Hide resolved
src/wrappers.jl Outdated Show resolved Hide resolved
src/wrappers.jl Outdated Show resolved Hide resolved
@visr
Copy link
Member

visr commented Jan 29, 2023

should the types go in a module they can be easily imported, instead of all exported

My first thought would be to not export them as they will often clash with other packages. Putting them in a module works, perhaps combined with making them available unexported in the GeoInterface namespace as well, such that GI.Point also works.

Can you elaborate a bit on how/where/why you want to use these wrapper types? Is it still mainly to make it easier to work with Base types that we consider to be GeoInterface compatible objects, and arrays thereof, as mentioned in the first post? Maybe the answer could go straight to the docs ;)

@rafaqz
Copy link
Member Author

rafaqz commented Jan 30, 2023

Reasoning:

  1. Tests. We are defining objects everywhere for tests but they are rarely flexible enough to do real testing of all the edge cases.
  2. Types that are missing in packages. Not every package implements everything, and wrappers can be used to fill those gaps
  3. Users cobbling together arbitrary geometries into objects from base points or any other geometries - even mixed together. This is currently really hard. How do we just make a LineString out of any kind of point? you have to learn it again and again for every package.
  4. Packages cobbling together objects for internal users e.g. MultiPolygons from polygons. How can they currently do that generically for any GeoInterface.jl compatible polygons without a dependency?
  5. Creating arbitrary features and feature collections. Not possible in half the packages and very useful to have a general method, see e.g. Allow plotting Vectors of geometries with GeoInterfaceMakie.jl #79

@visr
Copy link
Member

visr commented Jan 30, 2023

Thanks! Would you mind adding this to the docs as well? Perhaps also including when it's not needed/helpful to use these? For packages like GeoJSON and GeometryBasics I guess there is not directly much benefit from these, or not?

@rafaqz
Copy link
Member Author

rafaqz commented Jan 30, 2023

Yeah, I will add that.

Its actually pretty beneficial to GeometryBasics.jl as it has no Feature/FeatureCollection concept, again see this for a use case #79.

Also most packages (e.g. Rasters.jl) don't know about these packages or depend on them, so would still use the wrapper type over the package type as its generic. Even if GeoJSON has its own types how would we know about them without a dependency, and why would we write anything package specific when we don't have to.

I imagine it will be the same for users. Just learn one set of wrappers and use them everywhere, even if there are native types why bother learning that.

(this generality is made more useful with the PRs I'm making to ArchGDAL/LibGEOS etc - they don't care about geometry types anymore anything will work)

src/fallbacks.jl Outdated Show resolved Hide resolved
@rafaqz
Copy link
Member Author

rafaqz commented Feb 21, 2023

This is good to go, with a section in the docs.

I simplified it to just include the main types needed for e.g. LibGEOS tests.

We can work through issues (like with how to construct Triangle) later on, best to get this merged before I run out of steam to finish this and the PRs depending on it.

@rafaqz
Copy link
Member Author

rafaqz commented Mar 11, 2023

Coverage is up at 98%, and it seems like everthing makes sense - using it in LibGEOS tests seems to work well.

@visr @evetion can I get an approval to merge this?

@evetion
Copy link
Member

evetion commented Mar 11, 2023

🎉 I'll try to review tomorrow

Copy link
Member

@evetion evetion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gargantuan effort, very meta!

test/wrappers.jl Outdated Show resolved Hide resolved
test/wrappers.jl Outdated Show resolved Hide resolved
src/wrappers.jl Outdated Show resolved Hide resolved
src/wrappers.jl Show resolved Hide resolved
src/wrappers.jl Outdated Show resolved Hide resolved
src/wrappers.jl Outdated Show resolved Hide resolved
src/wrappers.jl Outdated Show resolved Hide resolved
src/wrappers.jl Show resolved Hide resolved
src/wrappers.jl Show resolved Hide resolved
src/wrappers.jl Outdated Show resolved Hide resolved
@evetion
Copy link
Member

evetion commented Mar 13, 2023

Closes #75

@rafaqz
Copy link
Member Author

rafaqz commented Mar 13, 2023

@evetion thanks for the super-thorough review, gives me a lot of confidence in quality of the PR and the interface generally.

It made me think we should aim for 100% coverage and this degree of correctness now that we are not rushing to just get the basics working.

@rafaqz
Copy link
Member Author

rafaqz commented Mar 26, 2023

Ok this has 100% coverage.

@evetion want to give this one last review? then we can squash and merge.

(it really needs squashing, there are a lot of trash commits here)

@evetion evetion merged commit 0c19f85 into main Mar 27, 2023
@evetion evetion changed the title add wrapper types Add wrapper types for all geometries Mar 27, 2023
@evetion evetion deleted the wrapper_types branch March 27, 2023 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants